Skip to content

Fiber: hold the object a callable resolved to in the creating frame - #24134

Merged
Girgias merged 3 commits into
php:PHP-8.4from
true-async:fiber-callable-object-8.4
Oct 14, 2026
Merged

Girgias merged 3 commits into
php:PHP-8.4from
true-async:fiber-callable-object-8.4

Conversation

@EdmondDantes

Copy link
Copy Markdown
Contributor

Fix use-after-free when a Fiber callable resolves to the creating method's $this

Problem

Inside a method of a class, [A::class, 'm'] and "A::m" resolve to a call of m() on the current $this. Fiber::__construct() stores that object in fci.object and fci_cache.object but takes no reference to it. If the object dies before Fiber::start(), the fiber calls the method on freed memory.

<?php
class A {
    public $name = "a1";
    public function m() { echo "m on {$this->name}\n"; }
    public function make() { return new Fiber([A::class, 'm']); }
    public function __destruct() { echo "released\n"; }
}

$fiber = (new A)->make();   // nothing references the A after this line
$fiber->start();

PHP 8.3.6 and PHP-8.4 print released before m on a1: the destructor has run and the object is freed when m() executes. Valgrind reports invalid reads. A new object allocated between make() and start() can take the freed slot.

Cause

Fiber::__construct() (Zend/zend_fibers.c) keeps a reference only to the callable zval:

	fiber->fci = fci;
	fiber->fci_cache = fcc;

	// Keep a reference to closures or callable objects while the fiber is running.
	Z_TRY_ADDREF(fiber->fci.function_name);

For [A::class, 'm'] the zval holds a class name and a method name, not the object. The object comes from the calling frame's $this, so nothing keeps it alive.

On PHP-8.4 a fiber suspended inside m() hits the same bug: if the last reference to the object goes while the fiber is suspended, m() resumes on freed memory.

Fix

  • Fiber::__construct() adds a reference to fci.object whenever it is set, for every kind of callable, so the release and the GC report need no condition.
  • zend_fiber_release_callable() releases the callable zval and that object together, after the call returns or when an unstarted fiber is freed.
  • zend_fiber_object_gc() reports the object, so a fiber stored in a property of the same object is a collectable cycle.

Not covered: a callable that resolves to __call is resolved again at start(), against the frame that calls start(). That is a separate issue.

Tests

Zend/tests/fibers/fiber-callable-object-outlives-caller.phpt covers five cases:

  • the fiber starts after its creator's last reference went;
  • the string form "A::m";
  • a fiber that is never started, released with its object;
  • a fiber suspended while its object has no other reference;
  • a cycle through the fiber, which gc_collect_cycles() collects.

Verified on a --disable-all --enable-debug build of PHP-8.4:

  • without the fix, the test fails and valgrind reports invalid reads;
  • with the fix, all 90 tests in Zend/tests/fibers pass and the new test is clean under valgrind (run-tests.php -m).

The same change on master passes the 111 tests in Zend/tests/fibers.

Inside a method of A, new Fiber([A::class, 'm']) or new Fiber("A::m")
resolves the callable to the method's $this. Fiber::__construct() kept
that object in fci.object and fci_cache.object without a reference, so
once the A died before start(), the fiber called A::m() on freed memory.

The fiber now holds fci.object for every callable, releases it with the
callable (after the call, or when the fiber is freed unstarted) and
reports it to the GC, so a fiber stored in a property of that object is
a collectable cycle.

A __call trampoline is not covered: it is resolved again at start(),
against the frame that calls start().

@Girgias Girgias left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks correct, could just just extract the logic into some functions? As it would make rebasing #23994 easier.

Comment thread Zend/zend_fibers.c Outdated
Comment thread Zend/zend_fibers.c Outdated
@EdmondDantes

Copy link
Copy Markdown
Contributor Author

This looks correct, could just just extract the logic into some functions? As it would make rebasing #23994 easier.

Done. Thank you

@Girgias

Girgias commented Oct 13, 2026

Copy link
Copy Markdown
Member

Seems there is a conflict, mind rebasing?

@EdmondDantes

Copy link
Copy Markdown
Contributor Author

Seems there is a conflict, mind rebasing?

@Girgias
I don't see a conflict with PHP-8.4: the branch merges into it cleanly, and GitHub reports it as mergeable. The only conflict I get is in NEWS when merging up into PHP-8.5 and master. Is that the one you mean, or something else?

@Girgias

Girgias commented Oct 14, 2026

Copy link
Copy Markdown
Member

Seems there is a conflict, mind rebasing?

@Girgias I don't see a conflict with PHP-8.4: the branch merges into it cleanly, and GitHub reports it as mergeable. The only conflict I get is in NEWS when merging up into PHP-8.5 and master. Is that the one you mean, or something else?

GitHub was showing it as having a conflict and thus non-mergeable.... so I guess it was just drunk.

@Girgias
Girgias merged commit 67decbe into php:PHP-8.4 Oct 14, 2026
17 of 18 checks passed
Girgias added a commit that referenced this pull request Oct 14, 2026
* PHP-8.6:
  Update NEWS
  ext/standard/tests: work around Windows quoting with tempfiles.
  .github/scripts/windows: pass an extension_dir to the test runner.
  {ext/standard,sapi/cli}/tests: pass TEST_PHP_EXTRA_ARGS to php
  ext/standard/tests: pass TEST_PHP_EXTRA_ARGS to some proc_open() tests
  sapi/cli/tests/023.phpt: support TEST_PHP_EXTRA_ARGS
  std: Fix memory leak of the last response headers after a nested HTTP request (#24201)
  Fiber: hold the object a callable resolved to in the creating frame (#24134)
Girgias added a commit to Girgias/php-src that referenced this pull request Oct 14, 2026
* PHP-8.4:
  Update NEWS
  ext/standard/tests: work around Windows quoting with tempfiles.
  .github/scripts/windows: pass an extension_dir to the test runner.
  {ext/standard,sapi/cli}/tests: pass TEST_PHP_EXTRA_ARGS to php
  ext/standard/tests: pass TEST_PHP_EXTRA_ARGS to some proc_open() tests
  sapi/cli/tests/023.phpt: support TEST_PHP_EXTRA_ARGS
  std: Fix memory leak of the last response headers after a nested HTTP request (php#24201)
  Fiber: hold the object a callable resolved to in the creating frame (php#24134)
Girgias added a commit to Girgias/php-src that referenced this pull request Oct 14, 2026
* PHP-8.5:
  Update NEWS
  ext/standard/tests: work around Windows quoting with tempfiles.
  .github/scripts/windows: pass an extension_dir to the test runner.
  {ext/standard,sapi/cli}/tests: pass TEST_PHP_EXTRA_ARGS to php
  ext/standard/tests: pass TEST_PHP_EXTRA_ARGS to some proc_open() tests
  sapi/cli/tests/023.phpt: support TEST_PHP_EXTRA_ARGS
  std: Fix memory leak of the last response headers after a nested HTTP request (php#24201)
  Fiber: hold the object a callable resolved to in the creating frame (php#24134)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants